chore(assets-controllers): add isDeprecated to TokenDetectionController - #9362
Conversation
Add an optional isDeprecated constructor callback so hosts can disable token detection when AssetsController supersedes this controller via the assets-unify-state feature flag. When deprecated, polling is stopped and all detection entry points become no-ops without tearing down the controller. Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
…roller tests Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
|
@cursor get latest and resolve merge conflicts |
|
Merged latest Conflict: Resolution: Kept both:
Pushed merge commit   |
…553e Resolve CHANGELOG.md conflict by keeping both the isDeprecated entry and latest main Unreleased changes. Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
|
Merged latest |
| * @param options.isDeprecated - Optional function that returns true to completely | ||
| * disable this controller (no requests, no token detection). The function is | ||
| * evaluated dynamically on each entry point so it can be toggled at runtime. | ||
| * Intended for use when a higher-level controller (e.g. AssetsController) | ||
| * supersedes this one. |
There was a problem hiding this comment.
@cursor keep this comment addition 1 short line, too long and want to keep diff short
There was a problem hiding this comment.
This is already done on the branch from the last review pass (e905f2de6).
The @param options.isDeprecated JSDoc is now a single line:
* @param options.isDeprecated - Optional callback that disables token detection when it returns true.The #enforceDisabledState JSDoc block was also removed. No further changes needed for this comment.
| /** | ||
| * Stops polling and blocks network activity so no token detection runs. | ||
| * | ||
| * Called from every entry point when `isDeprecated()` is true so that a | ||
| * runtime toggle propagates immediately, even if the controller was | ||
| * originally constructed while it was enabled. | ||
| */ |
There was a problem hiding this comment.
@cursor remove comment, want to keep comment short.
There was a problem hiding this comment.
Shortened the comments per review:
@param options.isDeprecated— reduced to one line:Optional callback that disables token detection when it returns true.#enforceDisabledState— removed the JSDoc block entirely
Pushed in e905f2de6.
Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
| this.#stopPolling(); | ||
| this.#disabled = true; |
There was a problem hiding this comment.
@cursor you didn't follow the pattern, we are meant to return state, not this shit. basically we want to return an empty state.
Example other controller deprecation
#enforceDisabledState(): void {
if (
Object.keys(this.state.accountsAssets).length === 0 &&
Object.keys(this.state.assetsMetadata).length === 0 &&
Object.keys(this.state.allIgnoredAssets).length === 0
) {
return;
}
this.update((state) => {
state.accountsAssets = {};
state.assetsMetadata = {};
state.allIgnoredAssets = {};
});
}So we want to do this, and basically bail early.
There was a problem hiding this comment.
Updated #enforceDisabledState to match the shared deprecation pattern:
#enforceDisabledState(): void {
this.#stopPolling();
if (Object.keys(this.state).length === 0) {
return;
}
this.update(() => ({}));
}Changes:
- Removed
#disabled = truemanipulation - Reset state via
this.update()with an early return when already empty (forTokenDetectionController, state is always{}) - Kept
#stopPolling()so active intervals stop when deprecated - Entry points still call
#enforceDisabledState()and bail early
Tests were updated to assert empty state / no side effects instead of isActive === false. All 8 isDeprecated tests pass.
Pushed in 059517e1f.
|
Updated |
…rceDisabledState Follow the shared isDeprecated pattern: clear state via this.update and bail early at entry points instead of toggling the private disabled flag. Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
GitHub Actions
Inactive
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 059517e. Configure here.
|
@cursor check these comments. My intention is we keep polling, but the state/update bails early (via the enforceDisabledState method). |
|
We weren't quite aligned before — Your intent (correct pattern, same as
Bugbot comments:
Fix pushed ( So yes — with that change, we're aligned with your intent. |
…s deprecated enforceDisabledState only clears state; entry points bail early without stopping legacy or mixin polling, matching the shared isDeprecated pattern. Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
|
Re: Bugbot comments — aligned with your intent now. Your intent: keep polling running; Were we good before? Not quite — Now:
Bugbot notes:
Test updated: |
|
@cursor get latest and resolve conflicts |
|
Merged latest Resolution: Kept both Unreleased entries: Also updated our changelog wording to match the current behavior (keep polling, bail early at entry points — no longer says polling is stopped). All 8 |
…553e Resolve CHANGELOG.md conflict by keeping both isDeprecated entries and updating the TokenDetectionController note to reflect keep-polling behavior. Co-authored-by: Prithpal Sooriya <prithpal.sooriya@gmail.com>
4 participants
Explanation
As part of the
assets-unify-staterollout,AssetsControllersupersedes legacy per-chain controllers.TokenDetectionControllerneeds the same opt-out mechanism already added toTokenListController,TokensController,TokenBalancesController, and related controllers.This PR adds an optional
isDeprecatedconstructor callback toTokenDetectionController. When it returnstrue, the controller stops polling, blocks network activity, and becomes a no-op at every entry point (start,detectTokens,_executePoll,addDetectedTokensViaWs, andaddDetectedTokensViaPolling). The callback is re-evaluated on each entry point so hosts can toggle deprecation at runtime without reconstructing the controller.References
Manual testing steps
N/A — behavior-preserving when
isDeprecatedis omitted (defaults tofalse).Screenshots/Recordings
N/A
Changed files
packages/assets-controllers/src/TokenDetectionController.tsisDeprecatedoption and#enforceDisabledState()guards on all detection entry pointsyarn eslinton changed filespackages/assets-controllers/src/TokenDetectionController.test.tsisDeprecatedtest suite (9 cases)yarn workspace @metamask/assets-controllers run jest --no-coverage packages/assets-controllers/src/TokenDetectionController.test.ts -t "isDeprecated"packages/assets-controllers/CHANGELOG.mdisDeprecatedoption under UnreleasedChecklist